Remove unnecessary xSemaphoreTake() of USBHID semaphore #7205
Conversation
|
@RefactorFactory - Thanks for the PR. Indeed, this extra @me-no-dev - Please take a look and let me know. Thanks. |
|
The reason for this call is to make sure that any previous given semaphore is cleared, so that the next Take will return after the current send is completed. Let's storm a bit on this first. Any chance you can provide a test case that would show an issue? |
What is the scenario when a given semaphore is left given / non-cleared? Do you mean a scenario like the following: This gist is a test case: https://gist.github.com/RefactorFactory/62dd9f98d40e55584966bed28fa98868
This output shows This test case demonstrates that the call to Thanks. |
|
Maybe it would make more sense if moved above |
I think I understand your alternative move, but what is the scenario where the previous semaphore is given, but not taken, that you're trying to address? One does not try to clear a critical section or mutex before entering it, so why clear a semaphore before trying to take it? What am I missing? Thanks. |
|
One option is to send a packet and not get the "sent" event on time, but there have been other cases where an extra "sent" will be received and the semaphore will already be given. The point of this semaphore is to make sending blocking, so send something, wait for the "sent" event and be able to send something else. If you have the semaphore given earlier and not cleared, function will return immediately and can cause USB overflow and other unintended consequences. |
The HID semaphore allows USBHID::SendReport() to wait for the completion of report sending. With a zero timeout xSemaphoreTake() after calling tud_hid_n_report(), occasionally, the following would happening: 1. USBHID::SendReport() would send a report by calling tud_hid_n_report(). 2. The send would complete and (presumably on another thread) tud_hid_report_complete_cb() would be called and it would xSemaphoreGive() the semaphore. 3. In USBHID::SendReport(), the zero timeout xSemaphoreTake(sem, 0) would succeed, taking the semaphore. 4. On the next line, xSemaphoreTake(sem, timeout_ms ...) would timeout because the semaphore was already taken by the previous line of code. The result would be waiting timeout_ms for no reason. The purpose of the zero timeout xSemaphoreTake() is to clear the semaphore in case a previous SendReport() timed out waiting for the semaphore. In that case, tud_hid_report_complete_cb() may be called after the timeout, giving the semaphore. Then the next SendReport() would start with the semaphore given, which isn't desired if we want to call xSemaphoreTake(sem, timeout_ms ...) on it. There have also been other cases where tud_hid_report_complete_cb() is called an extra time, causing the same situation. The fix is to move the zero timeout xSemaphoreTake() before the call to tud_hid_n_report(). This eliminates the race between the zero timeout xSemaphoreTake() and tud_hid_report_complete_cb() in the common case when no timeout occurs. There is still a possible race condition between the zero timeout xSemaphoreTake() and tud_hid_report_complete_cb() in the case of a timeout, but that should be rarer.
Ok, thanks for explaining, it makes sense. I'll update the commit to just move the zero timeout |
RefactorFactory
force-pushed
the
usbhid_semaphore
branch
from
d679b9b to
af1f2bd
Compare
Description of Change
The HID semaphore allows
USBHID::SendReport()to wait for the completion ofreport sending. There seemed to be an extra call to
xSemaphoreTake()of thissemaphore that served no purpose.
With this extra call, occasionally, the following would happening:
USBHID::SendReport()would send a report by callingtud_hid_n_report().tud_hid_report_complete_cb()would be called and it wouldxSemaphoreGive()the semaphore.
USBHID::SendReport(), the extraxSemaphoreTake(sem, 0)would succeed,taking the semaphore.
xSemaphoreTake(sem, timeout_ms ...)would timeoutbecause the semaphore was already taken by the previous line of code.
The result would be waiting
timeout_msfor no reason.The fix is to eliminate the extra unnecessary call to
xSemaphoreTake().Tests scenarios
I have tested my Pull Request on Arduino-esp32 core v2.0.4 with ESP32-S2 Board with this scenario.
While testing, I used
millis()to time how long the calls toxSemaphoreTake()would take.